-
-
Notifications
You must be signed in to change notification settings - Fork 854
Don’t generate invalid <link rel="canonical"/> with missing href
#3420
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don’t generate invalid <link rel="canonical"/> with missing href
#3420
Conversation
🦋 Changeset detectedLatest commit: 337c03a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for astro-starlight ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hello! Thank you for opening your first PR to Starlight! ✨ Here’s what will happen next:
|
4822238 to
40e2fd2
Compare
trueberryless
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for this and all the other awesome PRs.
These changes look good to me. While I couldn't find a resource stating that those tags are invalid without the content attribute, the semantic meaning of those tags is empty.
Here are the resources I found that only say how the tag is intended:
- https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/meta/name
- https://ogp.me/
- https://www.w3.org/TR/rdfa-primer/#using-the-content-attribute
While there is no problem using
linkin RDFa Lite (which uses thehrefattribute, i.e., can be used to define "flavored" links), the fact that, in a conforming HTML file, themetaelement may have no text content means that the only way of using the header for such statements is to use thecontentattribute.
Signed-off-by: Anders Kaseorg <[email protected]>
40e2fd2 to
7319a41
Compare
|
Adjusted as suggested, thanks. The specification for
For
|
delucis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to write some tests for this change to make sure we don’t accidentally regress. These can probably go in packages/starlight/__tests__/head/head.test.ts — we might need to update the getRouteDataTestContext and getTestHead helpers to allow for testing with site not set.
|
Thanks again for the PR @andersk! @erbierc has kindly offered to take over writing tests for this, so I’m going to merge this into an intermediate branch — https://github.com/withastro/starlight/tree/stack/invalid-canonical — so we can collaborate there before merging into |

Remove these invalid HTML tags from the output when
context.siteis unset:<link rel="canonical"/>with missinghref<meta property="og:url"/>with missingcontent